add SSL options to mssql #33071

stephane-airbyte · 2023-12-04T20:50:05Z

This is adding extra support for SSL options in our MS SQL source.

The MSSQL options are a bit different than postgres and MySql,so I'm not using their common logic. The flip side is that this change is not breaking compatibility!
We're basically adding the ability to send a certificate. (of the server or of the CA that signed the server certificate), and the ability to override the hostname used to connect in order to match the hostname of the server certificate.

While doing those changes, I also noticed that the password was actually mandatory in MsSqlSource, even though it wasn't marked as such in the spec.jsob.

The tests try to go the extra mile by connecting to the SQL server by IP rather than hostname, which triggers a failure to validate the certificate's host. And There's an extra test that connects by IP and passes the certificate's hostname in the spec, so the loop is closed.

On minor thing to note is that the MS SQL JDBC driver expects a keystore (technically a keystore file path), but we can't pass those through the current UI (because a key store is not a text file). So instead, we're passing a certificate, and the MsSqlSource converts that into a keystore file that is materialized in the container and its path is passed to the JDBC driver

tests are passing, connector version is bumpted

Resolves #30004

vercel · 2023-12-04T20:50:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jan 4, 2024 7:20pm

stephane-airbyte · 2023-12-04T20:50:16Z

Current dependencies on/for this PR:

master
- PR don't use MsSQLConfigBuilder.withSsl with strings, but instead properly named and typed methods #33261
  - PR fix MsSqlCdcSourceTest #33635
  - PR add SSL options to mssql #33071 👈

This stack of pull requests is managed by Graphite.

github-actions · 2023-12-04T20:50:27Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

stephane-airbyte · 2023-12-04T20:53:05Z

.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java

-      case "unencrypted" -> additionalParameters.add("encrypt=false");
+      case "unencrypted" -> {
+        additionalParameters.add("encrypt=false");
+        additionalParameters.add("trustServerCertificate=true");


Only added for clarity. By default, encrypt=false implies trustServerCertificate=true.

trustServerCertificate is trust all essentially.

This will trust any server certificate. I assume for the exchange of credentials which is still encrypted event when encrypt=false (you mentioned this offline)

stephane-airbyte · 2023-12-04T20:53:35Z

.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java

      case "encrypted_trust_server_certificate" -> {
        additionalParameters.add("encrypt=true");
        additionalParameters.add("trustServerCertificate=true");
      }
      case "encrypted_verify_certificate" -> {
        additionalParameters.add("encrypt=true");
+        additionalParameters.add("trustServerCertificate=false");


added for clarity. By default, encrypt=true implies trustServerCertificate=false

stephane-airbyte · 2023-12-04T20:55:31Z

.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java


        // trust store location code found at https://stackoverflow.com/a/56570588
-        final String trustStoreLocation = Optional


this was really not doing anything. If anything, it'd guarantee that the connection would fail (because with this, the sever certificate would have to be signed by one of the root authorities in the default trust store)

the old behavior is equivalent to not pushing anything as a trustStorelocation JDBC parameer

stephane-airbyte · 2023-12-04T20:56:52Z

.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java

+          char[] pwdArray = password.toCharArray();
+          final File trustStoreFile;
+          try {
+            trustStoreFile = File.createTempFile("mssqlTrustStoreFile", "jks");


The MSSQL driver takes a trust store file location as a parameter. Unfortunately, such a file is binary, which means it can't be uploaded from the current UI. It really doesn't matter, since a certificate is just as good, as long as we have either the server certificate that of the CA that signed the server certificate

stephane-airbyte · 2023-12-04T20:57:28Z

airbyte-integrations/connectors/source-mssql/src/main/resources/spec.json

@@ -4,7 +4,7 @@
    "$schema": "http://json-schema.org/draft-07/schema#",
    "title": "MSSQL Source Spec",
    "type": "object",
-    "required": ["host", "port", "database", "username"],
+    "required": ["host", "port", "database", "username", "password"],


the password is required in the parameter parsing logic in MssqlSource.java. Making it mandatory in the UI as well

Is this a breaking change? What is the expected behavior here?
I'm thinking of existing configs for example saved without a password

stephane-airbyte · 2023-12-04T20:58:08Z

airbyte-integrations/connectors/source-mssql/src/main/resources/spec.json

@@ -90,7 +90,7 @@
          {
            "title": "Encrypted (verify certificate)",
            "description": "Verify and use the certificate provided by the server.",
-            "required": ["ssl_method", "trustStoreName", "trustStorePassword"],
+            "required": ["ssl_method"],


unless I'm missing something, the previous required fields didn't even exist in the spec.json. Removing them...

stephane-airbyte · 2023-12-06T22:05:05Z

airbyte-integrations/connectors/source-mssql/src/test-integration/resources/dummy_config.json

@@ -3,4 +3,5 @@
  "port": 5555,
  "database": "default",
  "username": "default"
+  "password": "default"


now that the password is a mandatory field, it needs to be added here too

stephane-airbyte · 2023-12-06T22:06:37Z

...urce-mssql/src/testFixtures/java/io/airbyte/integrations/source/mssql/MsSQLTestDatabase.java

      super(testDatabase);
+      with(JdbcUtils.JDBC_URL_PARAMS_KEY, "loginTimeout=2");


this allows us to reduce the test running times from 1m to 2seconds for the tests that are expecting a failure to connect to the DB!

Do we always want to set this for all test cases, though? I don't have a problem with it. I took a more conservative approach with maybeSetShorterConnectionTimeout but that conservatism may not have been warranted.

no problems with it so far...

.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java

rodireich · 2023-12-07T23:27:52Z

.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java

-      case "unencrypted" -> additionalParameters.add("encrypt=false");
+      case "unencrypted" -> {
+        additionalParameters.add("encrypt=false");
+        additionalParameters.add("trustServerCertificate=true");


trustServerCertificate is trust all essentially.

This will trust any server certificate. I assume for the exchange of credentials which is still encrypted event when encrypt=false (you mentioned this offline)

postamar

I don't understand all of this but for what I do, I don't have any significant objections.

postamar · 2023-12-12T18:46:13Z

airbyte-integrations/connectors/source-mssql/metadata.yaml

@@ -9,7 +9,7 @@ data:
  connectorSubtype: database
  connectorType: source
  definitionId: b5ea17b1-f170-46dc-bc31-cc744ca984c1
-  dockerImageTag: 3.1.0
+  dockerImageTag: 3.1.1


The CI is going to complain about a missing release note.

This should probably go to the latest 3.5.0

postamar · 2023-12-12T18:47:43Z

...tors/source-mssql/src/test/java/io/airbyte/integrations/source/mssql/MssqlSslSourceTest.java

+  }
+
+  @ParameterizedTest
+  @EnumSource(CertificateKey.class)


Yesss! Though perhaps having two enums, one for valid and another for invalid certificates, might be preferable for expressing these tests.

postamar · 2023-12-12T18:51:18Z

...tors/source-mssql/src/test/java/io/airbyte/integrations/source/mssql/MssqlSslSourceTest.java

+        .build();
+    try {
+      AirbyteCatalog catalog = new MssqlSource().discover(config);
+      assertTrue(certificateKey.isValid);


Perhaps it's just me but I would find some branching logic if (certificateKey.isValid) { ... easier to read here. Do I understand correctly that we expect discover to throw iff !isValid?

no we expect discover to work for valid certificates, and fail for invalid ones. I will split this test in 2 to be clearer

postamar · 2023-12-12T18:53:00Z

...-mssql/src/testFixtures/java/io/airbyte/integrations/source/mssql/MsSQLContainerFactory.java

+  public void withSslCertificates(MSSQLServerContainer<?> container) {
+    // yes, this is uglier than sin. The reason why I'm doing this is because there's no command to
+    // reload a SqlServer config. So I need to create all the necessary files before I start the
+    // SQL server. Hence this horror


Whatever it takes!

postamar · 2023-12-12T18:54:31Z

...-mssql/src/testFixtures/java/io/airbyte/integrations/source/mssql/MsSQLContainerFactory.java

+        EOF
+        } && /opt/mssql/bin/sqlservr
+        """,
+        "{hostName}", container.getHost());


This is fine by me but perhaps using an env var to inject the host name would be simpler? Then again perhaps not, I don't know. Just a thought.

yeah, I guess that would work too. Even though I have more refactors in mind.

postamar · 2023-12-12T19:02:23Z

...urce-mssql/src/testFixtures/java/io/airbyte/integrations/source/mssql/MsSQLTestDatabase.java

      super(testDatabase);
+      with(JdbcUtils.JDBC_URL_PARAMS_KEY, "loginTimeout=2");


Do we always want to set this for all test cases, though? I don't have a problem with it. I took a more conservative approach with maybeSetShorterConnectionTimeout but that conservatism may not have been warranted.

airbyte-integrations/connectors/source-mssql/build.gradle

rodireich · 2023-12-21T04:17:44Z

airbyte-cdk/java/airbyte-cdk/core/src/main/resources/version.properties

@@ -1 +1 @@
-version=0.8.0
+version=0.8.1


make sure mssql points to the latest - 0.8.2(?)

Done (0.10.4)

rodireich

Thanks Stephane.
Just make sure all versions are aligned before merging,

airbyte-cdk/java/airbyte-cdk/README.md

postamar · 2024-01-04T18:58:52Z

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/7413665676
✅ Successfully published Java CDK version=0.10.4!

postamar · 2024-01-04T19:50:48Z

Updated and merged as discussed during team standup earlier this week.

Co-authored-by: Marius Posta <[email protected]>

octavia-squidington-iii added area/connectors Connector related issues connectors/source/mssql labels Dec 4, 2023

stephane-airbyte commented Dec 4, 2023

View reviewed changes

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch 3 times, most recently from bd13cbd to 2bee629 Compare December 6, 2023 22:00

stephane-airbyte commented Dec 6, 2023

View reviewed changes

stephane-airbyte requested review from postamar and rodireich December 6, 2023 22:06

stephane-airbyte marked this pull request as ready for review December 6, 2023 22:06

stephane-airbyte requested a review from a team as a code owner December 6, 2023 22:06

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch from 8764627 to e5150a9 Compare December 7, 2023 01:25

rodireich approved these changes Dec 7, 2023

View reviewed changes

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch from 6fe667a to 828771e Compare December 8, 2023 18:52

stephane-airbyte mentioned this pull request Dec 8, 2023

don't use MsSQLConfigBuilder.withSsl with strings, but instead properly named and typed methods #33261

Merged

postamar reviewed Dec 12, 2023

View reviewed changes

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch 2 times, most recently from e9d0acb to 19bf49f Compare December 19, 2023 17:27

stephane-airbyte mentioned this pull request Dec 19, 2023

fix MsSqlCdcSourceTest #33635

Closed

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch from 19bf49f to 366b329 Compare December 19, 2023 18:00

stephane-airbyte changed the base branch from master to stephane/12-19-move_mssql_to_latest_CDK_remove_LEGACY_state December 19, 2023 18:19

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch 3 times, most recently from 7901560 to d5e1af5 Compare December 21, 2023 00:31

stephane-airbyte requested a review from rodireich December 21, 2023 00:31

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Dec 21, 2023

vercel bot deployed to Preview December 21, 2023 00:36 View deployment

rodireich reviewed Dec 21, 2023

View reviewed changes

airbyte-integrations/connectors/source-mssql/build.gradle Outdated Show resolved Hide resolved

rodireich reviewed Dec 21, 2023

View reviewed changes

rodireich approved these changes Dec 21, 2023

View reviewed changes

fix MsSqlCdcSourceTest

ed91686

stephane-airbyte force-pushed the stephane/12-02-fix_MssqlSource.java branch from d5e1af5 to ed91686 Compare December 21, 2023 17:59

vercel bot deployed to Preview December 21, 2023 18:02 View deployment

katmarkham removed the request for review from a team January 3, 2024 18:17

Base automatically changed from stephane/12-07-don_t_use_MsSQLConfigBuilder.withSsl_with_strings_but_instead_properly_named_and_typed_methods to master January 4, 2024 17:52

postamar added 3 commits January 4, 2024 12:55

Merge branch 'master' into stephane/12-02-fix_MssqlSource.java

d24b1ab

bump CDK version required

7d2ee0a

bump source-mssql version

26302e1

postamar reviewed Jan 4, 2024

View reviewed changes

airbyte-cdk/java/airbyte-cdk/README.md Outdated Show resolved Hide resolved

Update airbyte-cdk/java/airbyte-cdk/README.md

a06a3f0

postamar added checklist-action-run and removed checklist-action-run labels Jan 4, 2024

chore: format code

7715617

useLocalCdk = false

e683c0c

postamar merged commit 15490f6 into master Jan 4, 2024
25 checks passed

postamar deleted the stephane/12-02-fix_MssqlSource.java branch January 4, 2024 19:50

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

add SSL options to mssql (airbytehq#33071)

3556efc

Co-authored-by: Marius Posta <[email protected]>

jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024

add SSL options to mssql (airbytehq#33071)

271b92f

Co-authored-by: Marius Posta <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add SSL options to mssql #33071

add SSL options to mssql #33071

stephane-airbyte commented Dec 4, 2023 •

edited by rodireich

Loading

vercel bot commented Dec 4, 2023 •

edited

Loading

stephane-airbyte commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 4, 2023 •

edited by postamar

Loading

stephane-airbyte Dec 4, 2023

rodireich Dec 7, 2023

stephane-airbyte Dec 4, 2023

stephane-airbyte Dec 4, 2023

stephane-airbyte Dec 4, 2023

stephane-airbyte Dec 4, 2023

stephane-airbyte Dec 4, 2023

rodireich Dec 19, 2023

stephane-airbyte Dec 4, 2023

stephane-airbyte Dec 6, 2023

stephane-airbyte Dec 6, 2023

postamar Dec 12, 2023

stephane-airbyte Dec 12, 2023

rodireich Dec 7, 2023

postamar left a comment

postamar Dec 12, 2023

rodireich Dec 21, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

stephane-airbyte Dec 12, 2023

postamar Dec 12, 2023

postamar Dec 12, 2023

stephane-airbyte Dec 12, 2023

postamar Dec 12, 2023

rodireich Dec 21, 2023

postamar Jan 4, 2024

rodireich left a comment

postamar commented Jan 4, 2024 •

edited by github-actions bot

Loading

postamar commented Jan 4, 2024


		// trust store location code found at https://stackoverflow.com/a/56570588
		final String trustStoreLocation = Optional

		super(testDatabase);
		with(JdbcUtils.JDBC_URL_PARAMS_KEY, "loginTimeout=2");

		@@ -1 +1 @@
		version=0.8.0
		version=0.8.1

add SSL options to mssql #33071

add SSL options to mssql #33071

Conversation

stephane-airbyte commented Dec 4, 2023 • edited by rodireich Loading

vercel bot commented Dec 4, 2023 • edited Loading

stephane-airbyte commented Dec 4, 2023 • edited Loading

github-actions bot commented Dec 4, 2023 • edited by postamar Loading

Before Merging a Connector Pull Request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodireich left a comment

Choose a reason for hiding this comment

postamar commented Jan 4, 2024 • edited by github-actions bot Loading

postamar commented Jan 4, 2024

stephane-airbyte commented Dec 4, 2023 •

edited by rodireich

Loading

vercel bot commented Dec 4, 2023 •

edited

Loading

stephane-airbyte commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 4, 2023 •

edited by postamar

Loading

postamar commented Jan 4, 2024 •

edited by github-actions bot

Loading